Skip to content

win: symbol exports and minor fixes #3024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
dhiltgen wants to merge 2 commits into
base: main
Choose a base branch
from
Open

win: symbol exports and minor fixes #3024

dhiltgen wants to merge 2 commits into from

Conversation

@dhiltgen
Copy link

@dhiltgen dhiltgen commented Jan 20, 2026

Proposed changes

This PR extracts the non-CUDA portions of #2972 for Windows support.

Test results (known GGUF library incompatibility on Windows)

(.venv) PS C:\Users\danie\code\mlx> .\build\tests\Release\tests.exe 
[doctest] doctest version is "2.4.12"
[doctest] run with "--help" for options
===============================================================================
C:\Users\danie\code\mlx\tests\load_tests.cpp(43):
TEST CASE:  test gguf

C:\Users\danie\code\mlx\tests\load_tests.cpp(43): ERROR: test case THREW exception: [save_gguf] Compile with MLX_BUILD_GGUF=ON to enable GGUF support.

===============================================================================
C:\Users\danie\code\mlx\tests\load_tests.cpp(97):
TEST CASE:  test gguf metadata

C:\Users\danie\code\mlx\tests\load_tests.cpp(97): ERROR: test case THREW exception: [save_gguf] Compile with MLX_BUILD_GGUF=ON to enable GGUF support.

===============================================================================
[doctest] test cases:  221 |  219 passed | 2 failed | 0 skipped
[doctest] assertions: 3045 | 3045 passed | 0 failed |
[doctest] Status: FAILURE!

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

This was referenced Jan 20, 2026
Open
Draft
zcbenz
zcbenz reviewed Jan 20, 2026
View reviewed changes
Copy link
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation generator does not seem to recognize the macro:

/home/runner/work/mlx/mlx/docs/src/cpp/ops.rst:6: WARNING: Error when parsing function declaration.
If the function has no return type:
  Error in declarator or parameters-and-qualifiers
  Invalid C++ declaration: Expecting "(" in parameters-and-qualifiers. [error at 8]
    MLX_API array roll (const array &a, const Shape &shift, const std::vector< int > &axes, StreamOrDevice s={})

mlx/random.cpp Outdated
T f = T(1.0);
uint16_t* m = (uint16_t*)&f;
*m -= 1;
f.bits_ -= 1;
Copy link
Collaborator

@zcbenz zcbenz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work on mac:

  /Users/runner/actions-runner/_work/mlx/mlx/mlx/random.cpp:92:4: error: member reference base type '__fp16' is not a structure or union
     92 |   f.bits_ -= 1;
        |   ~^~~~~~
  /Users/runner/actions-runner/_work/mlx/mlx/mlx/random.cpp:128:22: note: in instantiation of function template specialization 'mlx::core::random::below_one<__fp16>' requested here
    128 |         return array(below_one<float16_t>(), float32);
        |                      ^

Copy link
Author

@dhiltgen dhiltgen Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a good pattern for this one, so I backed it out. As a result, there's one known test failure:

C:\Users\danie\code\mlx\tests\random_tests.cpp(244):
TEST CASE:  test random uniform

C:\Users\danie\code\mlx\tests\random_tests.cpp(368): ERROR: CHECK( abs(float(mean(out).item<bfloat16_t>()) - 0.5f) < 0.02 ) is NOT correct!
  values: CHECK( 0.244141 <  0.02 )

Copy link
Collaborator

@zcbenz zcbenz Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce this failure on Mac running CPP tests with CPU, and it is also failing on Windows. /cc @awni

@zcbenz
Copy link
Collaborator

zcbenz commented Jan 20, 2026

Properly exporting public APIs on Windows requires adding a MLX_API macro before every public function, which is a common practice. I'm good with that but also /cc @awni @angeloskath @davidkoski since it would change how we define APIs.

zcbenz
zcbenz approved these changes Jan 21, 2026
View reviewed changes
Copy link
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for putting this together!

We still need a few more reviews before merging though.

benchmarks/cpp/CMakeLists.txt
set(target "${src_name}")
add_executable(${target} ${SRCFILE})
target_link_libraries(${target} PRIVATE mlx)
# On Windows, copy the mlx DLL to the executable directory for runtime loading
Copy link
Collaborator

@zcbenz zcbenz Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope there is a better solution but I'm good with what it is now. I have created a issue to track this: #3031.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zcbenz zcbenz zcbenz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dhiltgen @zcbenz